Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FIX] SlackBridge private channels #13261

Closed
wants to merge 18 commits into from
Closed

[FIX] SlackBridge private channels #13261

wants to merge 18 commits into from

Conversation

nylen
Copy link

@nylen nylen commented Jan 25, 2019 •
edited by MarcosSpessatto
Loading

Closes #13186
Closes #9408
Closes #11356
Closes #11485
Closes #7539
Closes #10002

Changed groups.info deprecated endpoint to conversations.info.
Changed to the new endpoint to get the members of room conversations.members.
Removed the deprecated message event file_share.

I think we can close #12213 in favor of this one.

We have a channel in our Slack group that was breaking the SlackBridge:

[34mI20190124-18:11:10.535(0) SlackBridge ➔ Rocket.debug Adding Rocket.Chat channel from Slack CD56XXXXX 
[34mI20190124-18:11:10.766(0) SlackBridge ➔ Rocket.debug Channel not added

This is a private channel, but its ID starts with C. This would already break the existing code if everything else worked, because the channel would be created as public in Rocket.Chat.

If that weren't enough of a problem, neither the channels.info nor the groups.info Slack API methods work with this channel:

curl 'https://slack.com/api/channels.info?token=xoxb-etc&channel=CD56XXXXX'
{"ok":false,"error":"method_not_supported_for_channel_type"}

curl 'https://slack.com/api/groups.info?token=xoxb-etc&channel=CD56XXXXX'
{"ok":false,"error":"channel_not_found"}

What does work is conversations.info. This doesn't have the channel.members key though, so we can get that with a separate call to conversations.members.

All other data elements needed by this area of the code are present in the conversations.info response as expected.

@CLAassistant
Copy link

CLAassistant commented Jan 25, 2019 •
edited
Loading

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@nylen nylen mentioned this pull request Jan 25, 2019
Closed
@nylen nylen force-pushed the fix/slackbridge-private-channels branch from 4efa23a to 1a960aa Compare January 25, 2019 06:59
MarcosSpessatto
MarcosSpessatto reviewed Mar 20, 2019
View reviewed changes
packages/rocketchat-slackbridge/server/RocketAdapter.js Outdated

slackResults = HTTP.get('https://slack.com/api/conversations.info', { params: { token: this.slackBridge.apiToken, channel: slackChannelID } });
if (slackResults && slackResults.data && slackResults.data.ok === true) {
slackMembers = HTTP.get('https://slack.com/api/conversations.members', { params: { token: this.slackBridge.apiToken, channel: slackChannelID } });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the documentation about this endpoint, when the "limit" parameter is not provided, the default limit is 100 items. How are you dealing with it, if the channel has more than 100 members?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcosSpessatto this is a good point. It looks like the previous limitation was 1500 members rather than 100 members:

2019-03-20T16 04 56-05 00

The documentation at https://api.slack.com/methods/conversations.members does not state a hard limit, but recommends using a page size of 200. A for loop using the limit and cursor parameters should be sufficient to make this work for any number of members.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcosSpessatto I do not have a test installation that would be appropriate to resolve this, and it's unlikely that I'll have time to look at it further. Please feel free to update this PR.

@vwbusguy
Copy link

vwbusguy commented Apr 8, 2019

Oddly enough, I'm able to replicate this bug. I have a private channel in Slack with a channel ID that starts with "C" and the sync silently fails between Slack and Rocket. I thought it was a failure of Private channels in general, but it does seem specific to channels with IDs that happen to start with "C".

@MarcosSpessatto MarcosSpessatto changed the title [FIX] SlackBridge private channels [WIP][FIX] SlackBridge private channels Apr 12, 2019
@MarcosSpessatto MarcosSpessatto self-assigned this Apr 12, 2019
@MarcosSpessatto MarcosSpessatto changed the title [WIP][FIX] SlackBridge private channels [FIX] SlackBridge private channels Apr 16, 2019
@MarcosSpessatto MarcosSpessatto added this to the 1.1.0 milestone Apr 16, 2019
@Hudell Hudell assigned Hudell and unassigned MarcosSpessatto Apr 25, 2019
@Hudell Hudell mentioned this pull request Apr 27, 2019
Merged
@engelgabriel engelgabriel modified the milestones: 1.1.0, 1.0.0 Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcosSpessatto MarcosSpessatto MarcosSpessatto left review comments

@sampaiodiego sampaiodiego Awaiting requested review from sampaiodiego

Assignees

@Hudell Hudell

Labels
feat: slackbridge
Projects
None yet
Milestone
1.0.0
7 participants
@nylen @CLAassistant @vwbusguy @MarcosSpessatto @engelgabriel @Hudell @sampaiodiego